Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add c2s support #30

Closed
wants to merge 6 commits into from
Closed

Add c2s support #30

wants to merge 6 commits into from

Conversation

hpychan
Copy link
Contributor

@hpychan hpychan commented Aug 20, 2011

Here is my c2s support for xmpp-server.

I used my forked version of vows to test.

hpychan/vows

I added the -i features in vows to support different lib, so I can see my code coverage.

The test command is
vows -i lib test/*

As a result, I updated test_jid.js as well.

Please tell me your thought.

hpychan

@astro
Copy link
Member

astro commented Aug 26, 2011

You know we're in the unfortunate situation of having two forks implementing c2s server. I'm currently tending towards the @superfeedr branch, just because Julien had me look into it earlier. Remember: release early, release often, and talk. Refusing contributions really gives me a bad conscience.

Nevertheless I want to give you some feedback. It could be nitpicking, but I don't want to hand your pull request down without technical reasons.

  • Your commits are huge and have non-descriptive messages. git provides nice means of representing development in a comprehensible way. (Julien's aren't much better: b9b6eba)
  • Roster isn't spelled Roaster
  • IqExtender looks useful. What is it for? :-) On the other hand, I don't like custom code performing magic for just specific parts.
  • Your tests are appreciated! Maybe we could merge them independently?
  • mechanisms belong to SASL and I'd like to have that kind of distinguishably separate from the XMPP stuff. We shall distinguish between client and server cases here too.
  • Specific queries like roster requests should not be handled by the library but the full-fledged XMPP server that will use the code one day.
  • Julien has implemented SASL PLAINTEXT for the server side as well. The other (digest) mechanisms that don't pass passwords in clear text are more interesting though.
  • TLS support is also quite important. We're having trouble with that.
  • XmppServer is named a bit unfortunate when it is c2s-specific. I think even the s2s-specific Router I added before is a misnomer.
  • Your code looks good, though style differs from the rest. I know that's a highly opinionated topic: I myself still have to convince emacs not to use tabs.
  • Would you mind explaining Logitech Connect? We've already got support for Facebook authentication, so I totally don't oppose such things.

@hpychan
Copy link
Contributor Author

hpychan commented Aug 26, 2011

Hi astro,

In my design, I want to allow anyone to extend code easily. I am not a believer on comments, but a strong believer on TDD. As a result, I got my code fullly coverage, yet might not have a good description. :)

Let me address your comments.

  1. I will add more descriptive message
  2. Rename to Roster should be a easy changes
  3. The intention of IqExtender is to allow other developers to add more extension for IQ, for example Roaster, IQ Query Action Protocol, and etc
  4. I am happy you like the tests. I can migrate the tests to you once you decide which c2s branch you want to merge into your code.
  5. My design of mechanisms also have your same intention mind. I would like to allow the C2S server to only support SASL mechanisms they want. You can write a Digest SASL mechanism as well.
  6. Roster can be removed as long as no iq extension is specified in constructor of XmppServer (C2SServer after change)
  7. PLAINTEXT or Digest mechanism should be support by the library for marshalling purpose, the actual validation should be extensible by the mechansim, which can be a web service/database query.
  8. I look into the Connection classes, I thought TLS has already support. I need to test it
  9. I will rename it to C2SServer?
  10. You like the code in tabs or spaces? I can follow your standard.
  11. Logitech Connect is a iq extension that I want to work with. I don't think it should be part of the node-xmpp.

@superfeedr
Copy link

Hey Hpychan, I think our c2s branch is now functional and works fine. However, one thing that it lacks is tests. I was hoping that maybe we could re-use the ones you used for your own branch.
I am not very familiar with vowsjs, but maybe you could point me to where I can start? Thanks!

@astro
Copy link
Member

astro commented Sep 15, 2011

Julien's branch has been merged now. Please help me maintaining it. :-)

@astro astro closed this Sep 15, 2011
sonnyp added a commit that referenced this pull request Dec 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants